refactor: removed dead code identified in issue - #848
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. WalkthroughThe pull request removes obsolete validation and fallback branches, propagates dispute setup failures, and replaces disputes-table rebuilding with transactional removal of legacy token columns. ChangesApplication and database cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The current changes are merge-ready after normal checks and review; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db.rs (1)
3410-3416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive-path migration test.
With
rebuild_disputes_table_preserves_rowsgone, the only remaining coverage is the no-op branch — the actual drop path (and row preservation) is untested. A test that adds the legacy columns back, inserts a row, runs the migration, and asserts both columns are gone while the row survives would keep coverage on the reachable logic this PR is consolidating onto.🧪 Sketch of the missing test
#[tokio::test] async fn migrate_remove_token_columns_drops_legacy_columns_and_preserves_rows() { let pool = migrated_pool().await; sqlx::query("ALTER TABLE disputes ADD COLUMN buyer_token INTEGER") .execute(&pool) .await .unwrap(); sqlx::query("ALTER TABLE disputes ADD COLUMN seller_token INTEGER") .execute(&pool) .await .unwrap(); // insert a dispute row here, then: migrate_remove_token_columns(&pool).await.unwrap(); assert!(!table_column_exists(&pool, "disputes", "buyer_token").await.unwrap()); assert!(!table_column_exists(&pool, "disputes", "seller_token").await.unwrap()); // assert the inserted row is still present }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 3410 - 3416, Add a positive-path test alongside migrate_remove_token_columns_is_noop_without_token_columns that restores both legacy token columns, inserts a disputes row, runs migrate_remove_token_columns, and verifies buyer_token and seller_token are removed while the inserted row remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/db.rs`:
- Around line 3410-3416: Add a positive-path test alongside
migrate_remove_token_columns_is_noop_without_token_columns that restores both
legacy token columns, inserts a disputes row, runs migrate_remove_token_columns,
and verifies buyer_token and seller_token are removed while the inserted row
remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46a3b8f6-0bd3-4fdf-b711-402423c284ff
📒 Files selected for processing (4)
src/app/admin_take_dispute.rssrc/app/dispute.rssrc/app/restore_session.rssrc/db.rs
💤 Files with no reviewable changes (1)
- src/app/restore_session.rs
ToRyVand
left a comment
ToRyVand
left a comment
There was a problem hiding this comment.
Reviewed the four changes against main. The substance is correct — both dead-code claims hold up under checking — but I found three things worth surfacing before merge.
Verified as genuinely unreachable (both claims correct):
restore_session.rs— the hex/length guards were dead.master_keyandtrade_keycome fromevent.identity/event.sender, which are alreadynostr_sdk::PublicKey;to_string()on those can only ever produce 64-char hex, so neither guard could fire. ThePublicKey::from_hexcalls further down insend_restore_session_response/send_restore_session_timeoutstill validate the string form where it actually matters, and their tests still cover the invalid-key path.admin_take_dispute.rs— theis_solver == 0recheck was structurally unreachable:find_solver_pubkeyqueriesWHERE pubkey == ?1 AND is_solver == trueviafetch_one, so anOkarm necessarily carriesis_solver == true.
Three findings:
1. The branch is 32 commits behind upstream/main and needs a rebase before merge. Side effect while reviewing: the suite here runs 1045 tests vs 1187 on current main, which initially looked like deleted test coverage and isn't — it's just the older base. Worth rebasing so a reviewer doesn't have to rule that out.
2. The SQLite justification reaches the right conclusion from the wrong premise. The commit message argues DROP COLUMN is safe by enumerating distro versions (Ubuntu 24.04 = 3.45, Bookworm = 3.40, Alpine 3.43+). But mostrod never uses the host's SQLite: libsqlite3-sys 0.37.0 compiles the bundled amalgamation (cargo:rerun-if-changed=sqlite3/sqlite3.c) and links it statically (cargo:rustc-link-lib=static=sqlite3), pinning SQLite 3.51.3 at compile time. So removing the fallback is safer than argued — it cannot depend on the deployment environment at all. Only flagging because that reasoning will be the record for whoever revisits this: the distro-version framing implies a host dependency that doesn't exist, and would send someone re-adding a fallback for a machine with old system SQLite.
3. The dispute.rs change is a behavior fix, not dead-code removal — you do say so in the description, so this is context for @grunch rather than a correction. Tracing it: setup_dispute returns Err(CantDoReason::DisputeCreationError) only when the disputing party's flag was already set. Old code skipped order.update() but kept going and still created the dispute row; new code returns early. Returning early is right.
The nuance: dispute_action already guards at the top with find_dispute_by_order_id(...).is_ok() → DisputeAlreadyExists, so the normal double-dispute path never reaches setup_dispute twice. That means the Err branch is only reachable from an inconsistent DB state (order flag set with no dispute row), where the old code silently self-healed and the new code hard-fails with DisputeCreationError — leaving that user unable to open a dispute until the state is fixed. I think failing loudly on a violated precondition is the correct call, and it's a state that shouldn't arise. Just worth a maintainer knowing it's the trade-off being made rather than discovering it from a report later.
Checks on the branch as-is: 1045 passed / 0 failed, cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean. The three migrate_remove_token_columns tests (no-op, both-columns, single-column) survive the rewrite, so the migration keeps its coverage.
Nothing here is a blocker on the code itself — my only actual ask is the rebase, plus optionally correcting the SQLite rationale so it doesn't mislead later. Contributor, not a maintainer, so this is a technical second opinion rather than a merge signal.
- restore_session: drop unreachable hex-validity guards on PublicKey values; nostr_sdk::PublicKey always serializes to 64-char hex so the guards could never trigger - admin_take_dispute: drop is_solver == 0 recheck after find_solver_pubkey; the SQL already filters WHERE is_solver == true so the Ok arm is structurally guaranteed non-zero - db: remove rebuild_disputes_table_without_tokens and the SQLite version-fallback branch inside migrate_remove_token_columns; DROP COLUMN is safe unconditionally - mostrod links libsqlite3-sys with \ the bundled SQLite source (3.51.3 compiled statically into the binary),\ so runtime behavior is independent of any host SQLite version - dispute: propagate setup_dispute error with map_err(MostroCantDo)? instead of silently swallowing it; the old if .is_ok() pattern skipped order.update() on failure but kept running, leaving the order flags unset while still creating the dispute row
9fc0299 to
69542b3
Compare
|
Thank you for the feedback @ToRyVand Also corrected the SQLite rationale in the commit message. The distro-version framing was wrong — mostrod links libsqlite3-sys 0.37.0 which compiles and statically links the bundled SQLite source (3.51.3) at build time, so the fallback removal is independent of any host SQLite version. Updated the message to reflect that. |
|
Verified independently against One note: rebase was onto |
cargo mutants left the `60 * 60` timeout computation untestable inline. Extract it as RESTORE_SESSION_TIMEOUT_SECS with a test that pins the value. The hex-validation extraction this commit originally carried is dropped: its only two call sites are the guards MostroP2P#848 removes as unreachable (`identity`/`sender` are `PublicKey`, so `.to_string()` is always 64 hex), and the two invalid-key tests it added already exist on main from MostroP2P#803.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Verified locally against current main (rebased onto d3f4804): merges clean, cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean, 1224 passed / 0 failed / 2 ignored.
All four items in #818 are addressed. I re-derived the two dead-code claims independently rather than trusting the description, and I'm not repeating @ToRyVand's three points — they're resolved or already on the record.
Independent confirmations
restore_session.rs—event.identityandevent.senderarePublicKey(non-Option) in mostro-core 0.14.5nip59.rs:71/75.to_string()can only yield 64-char hex, so both guards were unreachable. ✅admin_take_dispute.rs—find_solver_pubkey(db.rs:1404) filtersWHERE pubkey == ?1 AND is_solver == true, andusers.is_solverisinteger not null default 0, written only as 0/1. TheOkarm is structurallyis_solver == 1. ✅ The test doc-comment atadmin_take_dispute.rs:522already states the invariant, so coverage stays honest.db.rs— the legacy columns were plaininteger not null(41f182c), and there is no index, trigger, or view ondisputesanywhere inmigrations/. None of SQLite's remainingDROP COLUMNrestrictions (PK, UNIQUE, indexed, partial index, CHECK, FK, generated column, view, trigger) apply, so removing the fallback is safe on the merits. ✅
Findings
Four inline comments below. Only the missing test for the new dispute.rs error path is worth acting on inside this PR; the migrate_remove_token_columns reachability issue is pre-existing and belongs in its own issue.
Stale prior comment — no action needed
CodeRabbit's remaining nitpick asks for a positive-path migration test. It already exists: migrate_remove_token_columns_drops_legacy_columns_and_keeps_rows (src/db.rs:4794) recreates both legacy columns, inserts a dispute row, runs the migration, and asserts both columns are gone while the row survives. migrate_remove_token_columns_handles_single_legacy_column (src/db.rs:4822) covers the asymmetric case. Safe to resolve.
| /// and don't exist (newer databases). | ||
| /// Both drops run inside a single transaction so either both succeed or neither does. | ||
| /// This is a no-op when neither column exists (fresh installs and already-migrated databases). | ||
| async fn migrate_remove_token_columns(pool: &SqlitePool) -> Result<(), MostroError> { |
There was a problem hiding this comment.
MEDIUM | follow-up, not introduced by this PR
The drop path this PR keeps is itself unreachable in production.
migrations/20230928145530_disputes.sql was edited in place by #516 (ab338c5) to remove the two token columns. sqlx 0.9 Migrator::run calls validate_applied_migrations (sqlx-core-0.9.0/src/migrate/migrator.rs:255,274) and returns MigrateError::VersionMismatch whenever a recorded checksum differs from the file on disk.
So any database old enough to still carry buyer_token/seller_token fails at migrator.run(&conn) inside connect() — and connect() only recovers from duplicate column name errors via parse_duplicate_column_name / reconcile_existing_add_column_migration, never from VersionMismatch. migrate_remove_token_columns is therefore never reached on exactly the databases it exists to fix.
This PR is trimming ~140 lines from a function whose one remaining non-trivial branch is dead for the same class of reason as the branches #818 asked you to delete. Not a blocker and out of scope here, but worth an issue: either delete migrate_remove_token_columns and its tests entirely, or extend the existing reconciliation to handle VersionMismatch if pre-#516 databases are genuinely meant to be supported.
| } | ||
|
|
||
| /// Migrates legacy disputes table by removing deprecated buyer_token and seller_token columns if present. | ||
| /// Removes deprecated `buyer_token` and `seller_token` columns from the disputes table if present. |
There was a problem hiding this comment.
NIT | documentation accuracy
"Both drops run inside a single transaction so either both succeed or neither does" is accurate for the two ALTER TABLE statements, but the table_column_exists calls that decide what to drop run on a separate pooled connection, before pool.begin().
Harmless in practice — single caller, at startup, before the daemon serves anything — and unchanged from the previous implementation. Just worth not implying the whole read-decide-write sequence is atomic, since a future reader may rely on that claim.
| .await | ||
| .map_err(|cause| MostroInternalErr(ServiceError::DbAccessError(cause.to_string())))?; | ||
| } | ||
| order |
There was a problem hiding this comment.
LOW | test coverage
The new error path has no test. grep -rn DisputeCreationError src/ returns nothing.
This change turns a silently swallowed error into a user-visible MostroCantDo(CantDoReason::DisputeCreationError) response — a behavior change, as your description says — and nothing exercises it. The existing dispute_action_* tests cover NotFound, already-exists, non-disputable status, missing seller/buyer pubkey, non-party sender, and both happy paths; this new arm is the one gap.
It's reachable in a test by inserting an order with buyer_dispute = 1 and no row in disputes, then asserting dispute_action returns MostroCantDo(CantDoReason::DisputeCreationError) and that no dispute row was created — which also pins the intended "fail loudly on inconsistent state" semantics that @ToRyVand flagged as the trade-off being made here.
| } | ||
| order | ||
| .setup_dispute(is_buyer_dispute) | ||
| .map_err(MostroCantDo)?; |
There was a problem hiding this comment.
NIT | note for future readers
In mostro-core 0.14.5 (order.rs:537) setup_dispute assigns self.status = Status::Dispute.to_string() before the DisputeCreationError return, so on Err the local order is left dirty.
The new code returns immediately and never persists it, so this is strictly safer than the old if .is_ok() form. But anyone who later moves that ? or reuses order after this point inherits a trap. A one-line comment would make the invariant explicit:
// setup_dispute leaves order.status dirty on Err; we return before any persist.
order.setup_dispute(is_buyer_dispute).map_err(MostroCantDo)?;
MostroP2P#848 made a `setup_dispute` failure reach the client instead of being swallowed, but nothing pinned it: `DisputeCreationError` appeared nowhere under `src/`. The arm is only reachable from an inconsistent database state — an order whose dispute flag is set with no matching `disputes` row — since the ordinary double-dispute flow trips the `DisputeAlreadyExists` guard before `setup_dispute` runs twice. The test builds that state directly and asserts both the returned error and the side effect that actually changed: no dispute row is created. Also note the invariant at the call site: `setup_dispute` sets `order.status` before its error return, so the early return is what keeps the dirty value out of the database. Closes MostroP2P#907
3 participants
restore_session: drop unreachable hex-validity guards on PublicKey values; nostr_sdk::PublicKey always serializes to 64-char hex so the guards could never trigger
admin_take_dispute: drop is_solver == 0 recheck after find_solver_pubkey; the SQL already filters WHERE is_solver == true so the Ok arm is structurally guaranteed non-zero
db: remove rebuild_disputes_table_without_tokens and the SQLite version-fallback branch inside migrate_remove_token_columns; DROP COLUMN is supported on all deployment targets (SQLite >= 3.35 everywhere: Ubuntu 24.04 = 3.45, Debian Bookworm = 3.40, Alpine = 3.43+)
dispute: propagate setup_dispute error with map_err(MostroCantDo)? instead of silently swallowing it; the old if .is_ok() pattern skipped order.update() on failure but kept running, leaving the order flags unset while still creating the dispute row
Closes #818
Summary by CodeRabbit